Preserve Request headers in csrfFetch - Fixes #49#50
Conversation
When csrfFetch was called with a Request object as the first argument, any headers on the Request were stripped because only init?.headers were read. Merge Request headers, then init headers, then CSRF headers so existing headers survive and CSRF headers always take precedence. Fixes #49
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughFixes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/nuxt/src/runtime/utils/client.ts (1)
70-78: Header merge order is correct; consider adding matching Nuxt tests.The merge order (Request headers →
initheaders → CSRF headers) correctly preserves headers on aRequestinput while lettinginitoverride and CSRF take final precedence, matching the behavior and tests added for the Next.js package.However, the two new Request-based test cases were only added in
packages/nextjs/tests/client.test.ts. Since this file has the same behavior change, it'd be worth mirroring those tests here (e.g., underpackages/nuxt/tests/…) so the fix is regression-protected in both packages.Want me to port the two new Request-input test cases from
packages/nextjs/tests/client.test.tsinto the Nuxt test suite?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/runtime/utils/client.ts` around lines 70 - 78, Add Request-based unit tests in the Nuxt test suite mirroring the two new cases from packages/nextjs/tests/client.test.ts to ensure the header merge order in packages/nuxt/src/runtime/utils/client.ts (the Headers construction, the loop over init?.headers, and the loop over Object.entries(createCsrfHeaders(config))) is regression-protected; specifically port the tests that construct a Request input and assert headers preserve Request headers, allow init headers to override, and ensure CSRF headers (from createCsrfHeaders) take final precedence, placing them under packages/nuxt/tests with the same assertions and setup as the Next.js counterparts.packages/nextjs/src/client/client.ts (1)
152-160: Logic is correct; consider de-duplicating with the Nuxt implementation.The header construction correctly initializes from
input.headerswheninputis aRequest, mergesinit.headersviaset()(soinitwins overRequest), then applies CSRF headers last (so CSRF wins over both). This is the right precedence for the fix.Minor observation: this function is now byte-for-byte identical to
csrfFetchinpackages/nuxt/src/runtime/utils/client.ts(aside from theimport.meta.clientvs.typeof windowguard elsewhere). If there's a shared/core package in this monorepo, extractingcsrfFetch(and possiblycreateCsrfHeaders) there would avoid the two implementations drifting in future fixes like this one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/client/client.ts` around lines 152 - 160, The header-building logic in client.ts duplicates csrfFetch in Nuxt; extract the shared behavior into a common utility (e.g., createMergedHeaders or csrfFetch helper) and reuse it from both packages to prevent drift: move createCsrfHeaders (if not already core) and the header merge sequence (initializing from Request headers, then applying init.headers, then CSRF headers with set precedence) into a shared module, export a function with the same calling surface used by client.ts and csrfFetch in nuxt (referencing createCsrfHeaders and the header-merge logic), then replace the inline logic in client.ts and packages/nuxt/src/runtime/utils/client.ts with calls to the new shared function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/nextjs/src/client/client.ts`:
- Around line 152-160: The header-building logic in client.ts duplicates
csrfFetch in Nuxt; extract the shared behavior into a common utility (e.g.,
createMergedHeaders or csrfFetch helper) and reuse it from both packages to
prevent drift: move createCsrfHeaders (if not already core) and the header merge
sequence (initializing from Request headers, then applying init.headers, then
CSRF headers with set precedence) into a shared module, export a function with
the same calling surface used by client.ts and csrfFetch in nuxt (referencing
createCsrfHeaders and the header-merge logic), then replace the inline logic in
client.ts and packages/nuxt/src/runtime/utils/client.ts with calls to the new
shared function.
In `@packages/nuxt/src/runtime/utils/client.ts`:
- Around line 70-78: Add Request-based unit tests in the Nuxt test suite
mirroring the two new cases from packages/nextjs/tests/client.test.ts to ensure
the header merge order in packages/nuxt/src/runtime/utils/client.ts (the Headers
construction, the loop over init?.headers, and the loop over
Object.entries(createCsrfHeaders(config))) is regression-protected; specifically
port the tests that construct a Request input and assert headers preserve
Request headers, allow init headers to override, and ensure CSRF headers (from
createCsrfHeaders) take final precedence, placing them under packages/nuxt/tests
with the same assertions and setup as the Next.js counterparts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4f6b721-3419-4cea-b95a-3608fd748a3a
📒 Files selected for processing (3)
packages/nextjs/src/client/client.tspackages/nextjs/tests/client.test.tspackages/nuxt/src/runtime/utils/client.ts
1 participant
When csrfFetch was called with a Request object as the first argument, any headers on the Request were stripped because only init?.headers were read. Merge Request headers, then init headers, then CSRF headers so existing headers survive and CSRF headers always take precedence.
Fixes #49
Summary by CodeRabbit
Bug Fixes
Tests